Handle composite foreign keys in positioning scope changes#62
Merged
Conversation
In Rails 7.2+, destroyed_by_association.foreign_key returns an Array for composite foreign keys. The previous code compared the Array directly against individual scope column strings, which always returned false. This caused unnecessary position contraction when destroying a parent record through an association with composite keys. Also fix scope_columns setup in positioned() to properly flatten composite foreign keys into individual column entries. Agent-Logs-Url: https://github.com/brendon/positioning/sessions/18f96f84-12eb-4a58-ba20-4eaea20fe690 Co-authored-by: brendon <81871+brendon@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
brendon
March 27, 2026 00:42
View session
Owner
|
@copilot, Why is vendor/bundle/ruby/3.2.0 in the PR? |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
brendon
requested changes
Mar 27, 2026
Agent-Logs-Url: https://github.com/brendon/positioning/sessions/460cafb6-9237-459f-b203-8958a35847d8 Co-authored-by: brendon <81871+brendon@users.noreply.github.com>
Contributor
Author
brendon
reviewed
Mar 27, 2026
…model-based tests - Revert destroyed_via_positioning_scope? to brendon's && style - Remove .map(&:to_s) and .to_s calls (foreign keys are already strings) - Use << operator via Array().each for scope_columns in positioned - Replace mocked composite FK tests with proper model-based integration tests - Add CompositeKeyList/CompositeKeyListItem models for Rails 7.2+ testing Agent-Logs-Url: https://github.com/brendon/positioning/sessions/9ada62a9-2611-4113-9f17-e6c8f2ac9738 Co-authored-by: brendon <81871+brendon@users.noreply.github.com>
Agent-Logs-Url: https://github.com/brendon/positioning/sessions/9ada62a9-2611-4113-9f17-e6c8f2ac9738 Co-authored-by: brendon <81871+brendon@users.noreply.github.com>
- Use concat instead of .each { |fk| << fk } in positioning.rb as brendon requested
- Remove CompositeKeyList/CompositeKeyListItem models created from scratch
- Reuse existing CompositePrimaryKeyItem (which has composite PK) as parent
- Create minimal CompositeForeignKeyItem child with composite FK to it
Agent-Logs-Url: https://github.com/brendon/positioning/sessions/fd37d2c2-d391-4065-94bd-c2f48fe26b0a
Co-authored-by: brendon <81871+brendon@users.noreply.github.com>
Agent-Logs-Url: https://github.com/brendon/positioning/sessions/bc1c7fff-bca2-4f5d-bfb6-01f468d5e0da Co-authored-by: brendon <81871+brendon@users.noreply.github.com>
Agent-Logs-Url: https://github.com/brendon/positioning/sessions/9fee5710-2dda-4017-b269-daac0721916f Co-authored-by: brendon <81871+brendon@users.noreply.github.com>
brendon
requested changes
Mar 28, 2026
…omposite FK tests Agent-Logs-Url: https://github.com/brendon/positioning/sessions/ecfe22a7-dda1-4695-92fb-1ce97f9a57d4 Co-authored-by: brendon <81871+brendon@users.noreply.github.com>
Copilot
AI
changed the title
Handle composite foreign keys in destroyed_via_positioning_scope?
Remove Rails version skips and guards (now targeting Rails 7.2+)
Mar 28, 2026
brendon
requested changes
Mar 28, 2026
Agent-Logs-Url: https://github.com/brendon/positioning/sessions/3826dd80-2a5d-4ee8-8a22-a1d8efec9dd1 Co-authored-by: brendon <81871+brendon@users.noreply.github.com>
Contributor
Author
Copilot
AI
changed the title
Remove Rails version skips and guards (now targeting Rails 7.2+)
Handle composite foreign keys in positioning scope changes
Mar 28, 2026
brendon
reviewed
Mar 28, 2026
lib/positioning.rb
Outdated
|
|
||
| if reflection&.belongs_to? | ||
| positioning_columns[column][:scope_columns].concat Array(reflection.foreign_key) | ||
| positioning_columns[column][:scope_columns] << reflection.foreign_key |
Contributor
Author
There was a problem hiding this comment.
Restored in 6f04242 — now uses concat Array(reflection.foreign_key) to flatten composite FK columns into individual entries.
brendon
requested changes
Mar 28, 2026
…).include? in destroyed_via_positioning_scope?, remove unnecessary .flatten Agent-Logs-Url: https://github.com/brendon/positioning/sessions/8234fc70-91d7-47de-8d93-cc94bffa2f8d Co-authored-by: brendon <81871+brendon@users.noreply.github.com>
Contributor
Author
brendon
requested changes
Mar 28, 2026
…n: destroy child first, refute, then destroy parent, assert Agent-Logs-Url: https://github.com/brendon/positioning/sessions/7411e026-7446-4bbb-bfca-9e1f8f018683 Co-authored-by: brendon <81871+brendon@users.noreply.github.com>
brendon
approved these changes
Mar 28, 2026
Remove RAILS_VERSION-based conditional in Gemfile and unconditionally use gem 'sqlite3', '~> 2.9.0' when ENV['DB'] == 'sqlite'. This removes the older ~>1.7.3 fallback and simplifies sqlite dependency/version handling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since we now only test against Rails 7.2 and greater, this PR removes all version-conditional skips and guards across the test suite, and adds full support for composite foreign key scope changes.
Changes Made
if ActiveRecord.version >= Gem::Version.new("7.2.0")guard fromtest/support/active_record.rb— always create the composite_foreign_key_items tableif ActiveRecord.version >= Gem::Version.new("7.2.0")guard fromtest/models/composite_foreign_key_item.rb— always define the classif ActiveRecord.version >= Gem::Version.new("7.2.0")guard fromtest/models/composite_primary_key_item.rb— always define the has_manyskiplines fromtest/test_positioning.rb(3 × 7.2.0 skips, 1 × 7.1.0 skip)positioning_columns[column][:scope_columns] << reflection.foreign_keytopositioning_columns[column][:scope_columns].concat Array(reflection.foreign_key)inlib/positioning.rbso composite FK columns are stored flat (e.g.["cpki_item_id", "cpki_account_id"]instead of[["cpki_item_id", "cpki_account_id"]])destroyed_via_positioning_scope?inlib/positioning/mechanisms.rbto useArray(@positioned.destroyed_by_association.foreign_key).include?(scope_column)so it correctly handles both string and array foreign keysbelongs_to :composite_primary_key_itemtobelongs_to :list(withclass_name: "CompositePrimaryKeyItem") onCompositeForeignKeyItemso the model fits theTestPositioningabstract nomenclature where scope-change tests useupdate list: @second_list-> { order(:position) }tohas_many :composite_foreign_key_itemsonCompositePrimaryKeyItemTestCompositeForeignKeyPositioningnow extendsTestPositioningand inherits all positioning tests including scope-change tests — no no-op'd overridesTesting
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.